Restore 4 uniqueness removed in #998 where no xsd:key covers it (+ must-fail guards)#1032
Restore 4 uniqueness removed in #998 where no xsd:key covers it (+ must-fail guards)#1032thbar wants to merge 14 commits into
Conversation
(revert of 51b79ea for this constraint)
| </xsd:key> | ||
| <!-- =====GroupOfLinkSequences============================== --> | ||
| <!-- =====GroupOfLinkSequences unique========================== --> | ||
| <xsd:unique name="GroupOfLinkSequences_UniqueBy_Id_Version"> |
There was a problem hiding this comment.
Why do you think this is correct? For example, why isn't this just a key?
There was a problem hiding this comment.
See PR description (just added) ; I think it's more correct that what is in #998 on that specific bit, which was effectively removal of any sort of uniqueness check.
Why not key, well, key has been removed before v2.0.0 (see description). I don't know if adding it back in a patch release is a good idea or not, we'll have to discuss that collectively.
Also, assert that the file cannot be considered valid, first and foremost.
ValidBetween lost (afaik) id+version uniqueness in 51b79ea and no xsd:key covers it (guarded by examples/should-fail/duplicate-ValidBetween.xml). Exact verbatim revert, byte-identical to v2.0. Its selector also covers ValidityCondition/ValidityTrigger/ValidityRuleParameter, which keys already cover - kept verbatim to stay an exact revert, not trimmed (but this can be covered by a later optimisation).
ValidityPeriod lost id+version uniqueness in 51b79ea and no xsd:key covers it (guarded by examples/should-fail/duplicate-ValidityPeriod.xml). Should be verbatim revert (compared to v2.0). Its selector also covers UsageValidityPeriod, which a key already covers - kept verbatim to stay an exact revert, not trimmed.
| </xsd:key> | ||
| <!-- ===== UsageValidityPeriod Key ========================== --> | ||
| <!-- ===== UsageValidityPeriodting unique========================== --> | ||
| <xsd:unique name="UsageValidityPeriod"> |
There was a problem hiding this comment.
It should be unique by key.
There was a problem hiding this comment.
See PR description, I'm just undoing problematic removal of any uniqueness at all, this is not a PR where I'm changing any unique to a key (and as commented above, at least one was removed just pre-v2.0.0, so it would be a bit confusing).
To be discussed, then (but this is not this PR's goal).
There was a problem hiding this comment.
A duplicate id is never correct, it is a key. Hence it should not be unique.
|
@thbar read the comments. |
thbar
changed the title
| </xsd:key> | ||
| <!-- =====ValidityCondition============================== --> | ||
| <!-- =====ValidityCondition unique========================== --> | ||
| <xsd:unique name="ValidityCondition_UniqueBy_Id_Version"> |
There was a problem hiding this comment.
This is worth a dedicated ticket (for this one & all the places deserving a key, either removed erroneously before v2.0.0 or not), and its own separate PR(s).
#1032 is just short rollback of 4 missing uniques.
There was a problem hiding this comment.
I think the opposite is true. It was erronousley added as unique, and the removal script removed id unique, because the expectation was a key must exist for an id.
There was a problem hiding this comment.
Yes, yet the key isn't there, so it creates a larger trouble even (no more unicity at all). We'll need to address that, so I created #1034 ; we'll catalogue over there.
2 participants
Scope of this PR
xsd:keyenforces the uniqueness:GroupOfLinkSequences,OperatingDay,ValidBetweenandValidityPeriod.examples/should-fail/) that prove the uniqueness is no longer enforced — we can decide later what to do with them (e.g. keep them as permanent CI guards).This PR restores the missing
xsd:unique— otherwisev2.0.1would not be a correct optimisation, since it would silently drop validation thatv2.0.0had (these four were enforced inv2.0.0viaxsd:unique).It does not replace them with
xsd:key. ForGroupOfLinkSequences, afaikxsd:keywas there beforev2.0.0, but has removed by #721/#462 beforev2.0.0was released (which is why it's important to avoid also removing unique here).Whether to (re)introduce keys is a separate discussion we can have, but not the scope of this PR (and maybe not for a
v2.0.1bug fix either).